Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve attributes in several places #63468

Merged
merged 1 commit into from
Sep 9, 2019
Merged

Resolve attributes in several places #63468

merged 1 commit into from
Sep 9, 2019

Conversation

c410-f3r
Copy link
Contributor

@c410-f3r c410-f3r commented Aug 11, 2019

Resolve attributes for Arm, Field, FieldPat, GenericParam, Param, StructField and Variant.

This PR is based on @petrochenkov work located at petrochenkov@83fdb8d.

@petrochenkov
Copy link
Contributor

Meta: "draft" PRs don't interact well with our tooling, so it's better to avoid them.
"WIP" in the title is enough to see that the PR is an in progress work.

@rust-highfive

This comment has been minimized.

@c410-f3r c410-f3r changed the title Resolve attributes in several places [WIP] Resolve attributes in several places Aug 11, 2019
@c410-f3r c410-f3r marked this pull request as ready for review August 11, 2019 19:19
src/librustc_typeck/collect.rs Outdated Show resolved Hide resolved
src/librustc_typeck/collect.rs Outdated Show resolved Hide resolved
src/libsyntax/ast.rs Outdated Show resolved Hide resolved
src/libsyntax/ext/base.rs Outdated Show resolved Hide resolved
@petrochenkov petrochenkov added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 11, 2019
@bors

This comment has been minimized.

src/librustc/hir/mod.rs Outdated Show resolved Hide resolved
src/libsyntax/ext/base.rs Outdated Show resolved Hide resolved
src/libsyntax/visit.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

petrochenkov commented Aug 12, 2019

@c410-f3r
You can move the generic AST/HIR changes (Variant, new NodeIds) and visitor/mut_visitor changes into a separate PR, then we'll be able to merge them quickly.

@Centril
Copy link
Contributor

Centril commented Aug 12, 2019

(We'll need to feature gate using proc macro attrs on the new forms as well and set up a tracking issue unless there already is one.)

@petrochenkov
Copy link
Contributor

@Centril
The "expected an inert attribute, found an attribute macro" error in resolve currently works as a "feature gate" that cannot be enabled.

@Centril
Copy link
Contributor

Centril commented Aug 12, 2019

@petrochenkov I see; I was confused by the added // check-pass test (which shouldn't pass...).

@c410-f3r
Copy link
Contributor Author

You can move the generic AST/HIR changes (Variant, new NodeIds) and visitor/mut_visitor changes into a separate PR, then we'll be able to merge them quickly.

Indeed

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 13, 2019
Centril added a commit to Centril/rust that referenced this pull request Aug 14, 2019
syntax: Remove `DummyResult::expr_only`

The effect is that if a built-in macro both returns an erroneous AST fragment and is used in unexpected position, then the incorrect position error won't be reported.

This combination of two errors should be rare and isn't worth an extra field that makes people ask questions in comments.
(There wasn't even a test making sure it worked.)

Addresses rust-lang#63468 (comment)
r? @estebank
Centril added a commit to Centril/rust that referenced this pull request Aug 14, 2019
Add NodeId for Arm, Field and FieldPat

Extracted from rust-lang#63468
Centril added a commit to Centril/rust that referenced this pull request Aug 14, 2019
syntax: Remove `DummyResult::expr_only`

The effect is that if a built-in macro both returns an erroneous AST fragment and is used in unexpected position, then the incorrect position error won't be reported.

This combination of two errors should be rare and isn't worth an extra field that makes people ask questions in comments.
(There wasn't even a test making sure it worked.)

Addresses rust-lang#63468 (comment)
r? @estebank
Centril added a commit to Centril/rust that referenced this pull request Aug 14, 2019
expand: Unimplement `MutVisitor` on `MacroExpander`

Each call to `fully_expand_fragment` is something unique, interesting, and requiring attention.
It represents a "root" of expansion and its use means that something unusual is happening, like eager expansion or expansion performed outside of the primary expansion pass.
So, it shouldn't hide under a generic visitor call.

Also, from all the implemented visitor methods only two were actually used.

cc rust-lang#63468 (comment)
Centril added a commit to Centril/rust that referenced this pull request Aug 14, 2019
Add NodeId for Arm, Field and FieldPat

Extracted from rust-lang#63468
Centril added a commit to Centril/rust that referenced this pull request Aug 14, 2019
syntax: Remove `DummyResult::expr_only`

The effect is that if a built-in macro both returns an erroneous AST fragment and is used in unexpected position, then the incorrect position error won't be reported.

This combination of two errors should be rare and isn't worth an extra field that makes people ask questions in comments.
(There wasn't even a test making sure it worked.)

Addresses rust-lang#63468 (comment)
r? @estebank
@c410-f3r
Copy link
Contributor Author

c410-f3r commented Sep 9, 2019

@rustbot modify labels to +S-waiting-on-review , -S-waiting-on-author

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 9, 2019
src/test/ui/attrs-resolution-errors.rs Outdated Show resolved Hide resolved
src/test/ui/attrs-resolution.rs Outdated Show resolved Hide resolved
src/librustc_resolve/lib.rs Show resolved Hide resolved
src/librustc_resolve/build_reduced_graph.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

Great!
r=me with the remaining nits addressed and green CI

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 9, 2019
Arm, Field, FieldPat, GenericParam, Param, StructField and Variant
@c410-f3r
Copy link
Contributor Author

c410-f3r commented Sep 9, 2019

@petrochenkov Thanks for your patience

@Centril
Copy link
Contributor

Centril commented Sep 9, 2019

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Sep 9, 2019

📌 Commit 63a5f39 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 9, 2019
Centril added a commit to Centril/rust that referenced this pull request Sep 9, 2019
Resolve attributes in several places

Resolve attributes for Arm, Field, FieldPat, GenericParam, Param, StructField and Variant.

This PR is based on @petrochenkov work located at petrochenkov@83fdb8d.
bors added a commit that referenced this pull request Sep 9, 2019
Rollup of 5 pull requests

Successful merges:

 - #63468 (Resolve attributes in several places)
 - #64121 (Override `StepBy::{try_fold, try_rfold}`)
 - #64278 (check git in bootstrap.py)
 - #64306 (Fix typo in config.toml.example)
 - #64312 (Unify escape usage)

Failed merges:

r? @ghost
@bors bors merged commit 63a5f39 into rust-lang:master Sep 9, 2019
@petrochenkov
Copy link
Contributor

@c410-f3r
Thanks a lot, this was a longstanding issue and the last major missing part of macro modularization.

Centril added a commit to Centril/rust that referenced this pull request Sep 14, 2019
feature_gate: Remove dead code from attribute checking

rust-lang#63468 is merged, so all attributes go through name resolution now, so we can remove code that previously performed some checks for attributes not going through resolution.
bors added a commit that referenced this pull request Nov 16, 2019
Fully integrate derive helpers into name resolution

```rust
#[derive(Foo)]
#[foo_helper] // already goes through name resolution
struct S {
    #[foo_helper] // goes through name resolution after this PR
    field: u8
}
```
How name resolution normally works:
- We have an identifier we need to resolve, take its location (in some sense) and look what names are in scope in that location.

How derive helper attributes are "resolved" (before this PR):
- After resolving the derive `Foo` we visit the derive's input (`struct S { ... } `) as a piece of AST and mark attributes textually matching one of the derive's helper attributes (`foo_helper`) as "known", so they never go through name resolution.

This PR changes the rules for derive helpers, so they are not proactively marked as known (which is a big hack ignoring any ambiguities or hygiene), but go through regular name resolution instead.
This change was previously blocked by attributes not being resolved in some positions at all (fixed in #63468).

"Where exactly are derive helpers in scope?" is an interesting question, and I need some feedback from proc macro authors to answer it, see the post below (#64694 (comment)).
petrochenkov added a commit to petrochenkov/rust that referenced this pull request Feb 14, 2021
Starting from rust-lang#63468 cfg attributes on variants, fields, fn params etc. are processed together with other attributes (via `configure!`).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants